fix: prevent command injection via shell metacharacters on Windows by VenkatKwest · Pull Request #32678 · angular/angular-cli

VenkatKwest · 2026-03-04T10:48:31Z

Description

This PR fixes command injection vulnerabilities on Windows in two files:

1. `executor.ts` (git spawn)

Removed shell: true — git is a native .exe and doesn't need it
Switched from spawn(\git ${args.join(' ')}`)tospawn('git', args)`
Separated -m flag from commit message: ['commit', '-m', message] instead of ['commit', \-m "${message}"`]`

2. `host.ts` (package manager spawn)

Package managers (npm/yarn/pnpm) are .cmd scripts on Windows and need a shell
Instead of using shell: true with unsanitized argument concatenation, we now invoke cmd.exe /d /s /c directly with properly escaped arguments
Escape logic is based on cross-spawn's battle-tested approach, inlined to avoid adding a dependency
Uses windowsVerbatimArguments: true to prevent Node.js from re-escaping

Testing

Verified on Windows 11 (Node v25.6.0):

npm/git commands work correctly with the new approach
Injection payloads (& echo INJECTED, & echo PWNED > file) are blocked
Old shell: true pattern confirmed vulnerable (control test)

Fixes #32509

…revent command injection Git is a native executable on Windows and does not require shell: true. Switch to array-based spawn and separate the -m flag from the commit message to prevent command injection via crafted commit messages.

Escape shell metacharacters when invoking package managers via cmd.exe instead of using shell: true with unsanitized arguments. The escape logic is based on cross-spawn's approach of directly invoking cmd.exe with properly escaped arguments and windowsVerbatimArguments: true.

gemini-code-assist

Code Review

This pull request addresses command injection vulnerabilities on Windows by removing shell: true where it's not needed and by properly escaping arguments when a shell is required.

The changes in packages/angular_devkit/schematics/tasks/repo-init/executor.ts for git commands are correct, switching to spawn with an argument array instead of a concatenated string.

The changes in packages/angular/cli/src/package-managers/host.ts for package manager commands correctly identify the need for a shell on Windows but replace the insecure shell: true with a direct call to cmd.exe. However, I've found an issue in the custom argument escaping logic. The regular expressions for escaping backslashes are different from the cross-spawn library they are based on, and one of them appears to be buggy, which could lead to incorrect escaping. I've left a comment with a suggested fix to align the implementation with cross-spawn's battle-tested approach.

_{Note: Security Review is unavailable for this PR.}

packages/angular/cli/src/package-managers/host.ts

alan-agius4 · 2026-03-04T13:21:16Z

packages/angular/cli/src/package-managers/host.ts

+  // Convert to string
+  arg = `${arg}`;
+


This seems redundant since the input is a string.

Suggested change

// Convert to string

arg = `${arg}`;

alan-agius4 · 2026-03-04T13:27:43Z

packages/angular/cli/src/package-managers/host.ts

@@ -20,6 +20,42 @@ import { platform, tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { PackageManagerError } from './error';



The below code is literally a copy and paste from cross-spawn. Please add links or re-write and mention that is was inspires from. Otherwise this cannot be merged.

packages/angular/cli/src/package-managers/host.ts

alan-agius4

I had a chat with @clydin and actually this is not a vulnerability at all as the commit message cannot be passed via the command line and thus command injection cannot be triggered in this case.

Hence, kindly revert all the changes host.ts, can leave the cleanup in the executor.ts

Remove redundant string conversion, add proper attribution with authoritative Microsoft documentation link, and refactor to avoid multiple re-assignments as suggested by reviewer.

VenkatKwest · 2026-03-04T14:24:07Z

Thanks for the review, @alan-agius4! I've addressed all your feedback:

**Removed the redundant arg = \${arg}`** — since the parameter is already typed as string`.
Added proper attribution with source links — the docblock now clearly states it's adapted from cross-spawn's lib/util/escape.js and links to the authoritative Microsoft documentation for the escaping algorithm it uses.
Refactored to avoid multiple re-assignments — using a chained .replace() approach as you suggested.

VenkatKwest · 2026-03-05T03:15:27Z

bandicam.2026-03-05.11-17-49-918.mp4

bandicam.2026-03-05.11-23-12-736.mp4

Thanks @alan-agius4 for the review and for checking with @clydin. I appreciate the time you both took.

You're correct about executor.ts — I verified that --commit.message is rejected by the CLI's yargs parser (Unknown argument: commit.message), and --commit is only registered as a boolean. The commit message in executor.ts is not user-controlled via the CLI. I'm happy to keep that as a defense-in-depth cleanup only.

However, I respectfully disagree about host.ts. The host.ts vulnerability is real and exploitable. I have two confirmed PoCs where calc.exe opens on Windows:

PoC 1: Package specifier injection via `ng add`

ng add "lodash@1.0.0 & calc" --skip-confirmation

Result: calc.exe opens. The injected command executes silently alongside npm install.

Why it works — full stack trace:

IMPORTANT NOTE: STACK TRACE MIGHT BE WRONG BUT ABLE TO REPRODUCE THE VULN

1. User runs: ng add "lodash@1.0.0 & calc" --skip-confirmation

2. add/cli.ts:164 → npa("lodash@1.0.0 & calc")
   └─ npa splits at '@' → name="lodash", spec="1.0.0 & calc"
   └─ npa.js:462 → semver.validRange("1.0.0 & calc") returns truthy
   └─ npa classifies as type: "range"
      (BYPASSES the encodeURIComponent check on line 468,
       which only runs for type "tag")
   └─ Returns: { name: "lodash", type: "range", fetchSpec: "1.0.0 & calc" }

3. add/cli.ts → loadPackageInfoTask()
   └─ packageManager.getManifest(packageIdentifier)
   └─ package-manager.ts:494 → case 'range': getRegistryMetadata("lodash")
   └─ Fetches metadata, finds version "1.0.0" satisfies range
   └─ Returns manifest for lodash@1.0.0 ✓

4. add/cli.ts → installPackageTask()
   └─ packageManager.add("lodash@1.0.0 & calc", ...)
   └─ package-manager.ts:326 → args = ["add", "lodash@1.0.0 & calc", "--save-exact"]
   └─ package-manager.ts:327 → this.#run(args, { registry })

5. package-manager.ts:196 → host.runCommand("npm", ["add", "lodash@1.0.0 & calc", ...])

The key bypass is in npm-package-arg (npa). When a user provides lodash@1.0.0 & calc, npa splits at @ and passes 1.0.0 & calc to semver.validRange(). Semver is permissive — it ignores the trailing garbage & calc and returns a valid range. Because npa classifies this as type "range" (not "tag"), the encodeURIComponent validation on npa.js line 468 is never executed. The unsanitized & calc flows all the way to host.ts.

PoC 2: Registry flag injection via `ng add --registry`

ng add @angular/localize --registry "http://registry.npmjs.org & calc" --skip-confirmation

Result: calc.exe opens 3 times (once per npm command invoked during install).

Why it works — full stack trace:

1. User runs: ng add @angular/localize --registry "http://registry.npmjs.org & calc"

2. add/cli.ts:118 → yargs parses --registry as a string option (no validation)
   └─ options.registry = "http://registry.npmjs.org & calc"

3. add/cli.ts:555 → installPackageTask() destructures { registry } from options
   └─ add/cli.ts:583-590 → packageManager.add(packageIdentifier, ..., { registry })
   └─ package-manager.ts:327 → this.#run(args, options)  (options includes { registry })
   └─ package-manager.ts:161 → #run(args, { registry: "http://registry.npmjs.org & calc" })

4. package-manager.ts:172 → descriptor.getRegistryOptions(registry)
   └─ Returns: { args: ["--registry", "http://registry.npmjs.org & calc"] }
   └─ package-manager.ts:180 → finalArgs.push("--registry", "http://registry.npmjs.org & calc")

5. package-manager.ts:196 → host.runCommand("npm", [..., "--registry", "http://registry.npmjs.org & calc"])

6. host.ts:182 → shellCommand = "npm view ... --registry http://registry.npmjs.org & calc"

The host.ts fix is a critical security fix, not just a cleanup. I agree that executor.ts is defense-in-depth only.

Tested on: Angular CLI 21.1.4, Node.js v25.6.0, Windows 11, npm 11.3.0

Happy to adjust the PR scope based on this. Let me know how you'd like to proceed.

VenkatKwest added 2 commits March 4, 2026 16:13

angular-robot bot added area: @angular-devkit/schematics area: @angular/cli labels Mar 4, 2026

gemini-code-assist bot reviewed Mar 4, 2026

View reviewed changes

packages/angular/cli/src/package-managers/host.ts Outdated Show resolved Hide resolved

alan-agius4 reviewed Mar 4, 2026

View reviewed changes

packages/angular/cli/src/package-managers/host.ts Outdated Show resolved Hide resolved

alan-agius4 reviewed Mar 4, 2026

View reviewed changes

packages/angular/cli/src/package-managers/host.ts Show resolved Hide resolved

alan-agius4 requested changes Mar 4, 2026

View reviewed changes

refactor(@angular/cli): address review feedback for escapeArgForCmd

1b9dcc8

Remove redundant string conversion, add proper attribution with authoritative Microsoft documentation link, and refactor to avoid multiple re-assignments as suggested by reviewer.

alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 4, 2026

alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 5, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent command injection via shell metacharacters on Windows#32678

fix: prevent command injection via shell metacharacters on Windows#32678
VenkatKwest wants to merge 3 commits intoangular:mainfrom
VenkatKwest:fix/shell-injection-windows

VenkatKwest commented Mar 4, 2026

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

alan-agius4 Mar 4, 2026

Uh oh!

alan-agius4 Mar 4, 2026

Uh oh!

Uh oh!

Uh oh!

alan-agius4 left a comment

Uh oh!

VenkatKwest commented Mar 4, 2026

Uh oh!

VenkatKwest commented Mar 5, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -20,6 +20,42 @@ import { platform, tmpdir } from 'node:os';
		import { join } from 'node:path';
		import { PackageManagerError } from './error';

Conversation

VenkatKwest commented Mar 4, 2026

Description

1. executor.ts (git spawn)

2. host.ts (package manager spawn)

Testing

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

alan-agius4 Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

VenkatKwest commented Mar 4, 2026

Uh oh!

VenkatKwest commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PoC 1: Package specifier injection via ng add

PoC 2: Registry flag injection via ng add --registry

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. `executor.ts` (git spawn)

2. `host.ts` (package manager spawn)

VenkatKwest commented Mar 5, 2026 •

edited

Loading

PoC 1: Package specifier injection via `ng add`

PoC 2: Registry flag injection via `ng add --registry`